Refactore PropagateToMuon and add propagation to L3 muon filters at HLT by JanFSchulte · Pull Request #37180 · cms-sw/cmssw

JanFSchulte · 2022-03-08T19:56:00Z

Follow-up to #37086, adding the propagation of the L3 muon trajectory to the second muon station for the L3-L1 matching in a handful of HLT filters.

Out of the suggestions made by @makortel in that PR to improve the code, I opted to refactor the PropagateToMuon class into two classes.

We re-checked the HLT timing and as before we do not observe a significant change when we add the propgation (compare blue and black):

In terms of rates, no significant change is observed for higher-pt muon triggers, such as IsoMu24. Changes are observed for low-pT muon triggers, especially the Bs->mumu trigger HLT_DoubleMu4_3_Bs_v14, which goes from ~15 to ~22 Hz. We observe that the HLT efficiency for Bs->mumu candidates passing the analysis selection improves from ~75% to ~90%, so a good deal of the rate increase is coming from genuine Bs that BPH needs for analysis.

Full rate results are here: https://jschulte.web.cern.ch/jschulte/rates/

fyi @missirol @khaosmos93

cmsbuild · 2022-03-08T20:03:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28748

This PR adds an extra 76KB to repository

cmsbuild · 2022-03-08T20:03:53Z

A new Pull Request was created by @JanFSchulte (Jan-Frederik Schulte) for master.

It involves the following packages:

DQMOffline/L1Trigger (dqm, l1)
HLTrigger/Configuration (hlt)
HLTrigger/Muon (hlt)
L1Trigger/L1TNtuples (l1)
MuonAnalysis/MuonAssociators (analysis)

@Martin-Grunewald, @rekovic, @epalencia, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @pmandrik, @santocch, @cecilecaillol, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@HuguesBrun, @CeliaFernandez, @calderona, @silviodonato, @abbiendi, @jhgoh, @Martin-Grunewald, @bellan, @sscruz, @Fedespring, @thomreis, @dinyar, @battibass, @missirol, @rociovilar, @trocino, @cericeci, @kreczko this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Martin-Grunewald · 2022-03-09T05:52:02Z

HLTrigger/Configuration/python/customizeHLTforCMSSW.py

+	process.load("TrackPropagation.SteppingHelixPropagator.SteppingHelixPropagatorAny_cfi")
+	process.load("TrackPropagation.SteppingHelixPropagator.SteppingHelixPropagatorAlong_cfi")
+	process.load("TrackPropagation.SteppingHelixPropagator.SteppingHelixPropagatorOpposite_cfi") 
+	return process


Why is loading all these needed? Are these for various possible options but only one is used in the end?

The HLT menu already contains:

process.SteppingHelixPropagatorAny = cms.ESProducer( "SteppingHelixPropagatorESProducer", process.hltESPFastSteppingHelixPropagatorAny = cms.ESProducer( "SteppingHelixPropagatorESProducer", process.hltESPFastSteppingHelixPropagatorOpposite = cms.ESProducer( "SteppingHelixPropagatorESProducer", process.hltESPSteppingHelixPropagatorAlong = cms.ESProducer( "SteppingHelixPropagatorESProducer", process.hltESPSteppingHelixPropagatorOpposite = cms.ESProducer( "SteppingHelixPropagatorESProducer",

Perhaps some cleanup and rationalisation is needed!

I did make the label if the propagators configurable so the ones already in the menu can be used.
There is indeed some logic that chooses the propagator that is used in the code:

cmssw/MuonAnalysis/MuonAssociators/src/PropagateToMuonCore.cc

Lines 148 to 162 in 23e7243

const Propagator *propagatorBarrel = &*propagator_;

const Propagator *propagatorEndcaps = &*propagator_;

if (whichState_ != AtVertex) {

if (start.position().perp() > barrelCylinder_->radius())

propagatorBarrel = &*propagatorOpposite_;

if (fabs(start.position().z()) > endcapDiskPos_[useMB2_ ? 2 : 1]->position().z())

propagatorEndcaps = &*propagatorOpposite_;

}

if (cosmicPropagation_) {

if (start.momentum().dot(GlobalVector(start.position().x(), start.position().y(), start.position().z())) < 0) {

// must flip the propagations

propagatorBarrel = (propagatorBarrel == &*propagator_ ? &*propagatorOpposite_ : &*propagator_);

propagatorEndcaps = (propagatorEndcaps == &*propagator_ ? &*propagatorOpposite_ : &*propagator_);

}

}

From what I can see, only the Along and Opposite are actually being used, so the Any could in principle be left out.

cmsbuild · 2022-03-09T19:44:42Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28763

This PR adds an extra 36KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28763/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28763/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-03-09T19:56:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28764

This PR adds an extra 144KB to repository

cmsbuild · 2022-03-09T19:56:23Z

Pull request #37180 was updated. @Martin-Grunewald, @rekovic, @epalencia, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @jfernan2, @pmandrik, @santocch, @cecilecaillol, @pbo0, @rvenditti can you please check and sign again.

JanFSchulte · 2022-03-09T20:05:31Z

I should note that without this customization, the tests will fail because HLT crashes: https://github.com/cms-sw/cmssw/pull/37180/files#diff-8fc23ee531566af47b6eff1b354bb3db1c4e8f3925413edc3632e417d168b75fR158

missirol

Here's a first look from my side.

missirol · 2022-03-09T21:58:42Z

MuonAnalysis/MuonAssociators/interface/PropagateToMuonCore.h

-            Support for other muon stations will be added on request.
-
+  \class    PropagateToMuonCore PropagateToMuonCore.h
+  "HLTriggerOffline/Muon/interface/PropagateToMuonCore.h" \brief    Propagate an


Suggested change

"HLTriggerOffline/Muon/interface/PropagateToMuonCore.h" \brief Propagate an

\brief Propagate an

missirol · 2022-03-09T22:00:19Z

MuonAnalysis/MuonAssociators/interface/PropagateToMuonCore.h

-public:
-  explicit PropagateToMuon(const edm::ParameterSet &iConfig, edm::ConsumesCollector);
-  ~PropagateToMuon();
+//This is a nearly identical copy to MuonAnalysis/MuonAssociators/interface/PropagateToMuonCore.h


I'm not sure I understand this comment. Isn't this file the same that contains the comment?

There is a nearly identical copy of the PropagateToMuon class, for reasons I don't know, in HLTrigger/Offline, and I somewhat randomly used that one as the starting point for the refactoring, hence the leftover comment. I removed it.

missirol · 2022-03-09T22:03:08Z

MuonAnalysis/MuonAssociators/interface/PropagateToMuonInterface.h

+  object (usually a track) to the second muon station. Support for other muon
+  stations will be added on request.
+
+  \author   Giovanni Petrucciani


I would either remove the name(s) altogether, or add your name too to signal that the class has been reworked.

missirol · 2022-03-09T22:03:43Z

MuonAnalysis/MuonAssociators/interface/trackStateEnums.h

+#ifndef HLTriggerOffline_Muon_interface_trackStateEnums_h
+#define HLTriggerOffline_Muon_interface_trackStateEnums_h


missirol · 2022-03-09T22:07:38Z

MuonAnalysis/MuonAssociators/src/PropagateToMuonCore.cc

+  Chi2MeasurementEstimator estimator(1e10,
+                                     3.);  // require compatibility at 3 sigma


Suggested change

Chi2MeasurementEstimator estimator(1e10,

3.); // require compatibility at 3 sigma

// require compatibility at 3 sigma

Chi2MeasurementEstimator estimator(1e10, 3.);

Avoid wierd break by code-format by moving comment above line.

missirol · 2022-03-09T22:15:10Z

HLTrigger/Configuration/python/customizeHLTforCMSSW.py

+        filter.propagatorAny = cms.ESInputTag("", "SteppingHelixPropagatorAny")
+        filter.propagatorOpposite = cms.ESInputTag("", "hltESPSteppingHelixPropagatorOpposite")
+
+    return process


customizeFor37180 also needs to be called inside customizeHLTforCMSSW (the latter contains examples).

missirol · 2022-03-09T22:18:22Z

HLTrigger/Muon/plugins/HLTMuonTrkL1TFilter.h

                 trigger::TriggerFilterObjectWithRefs& filterproduct) const override;

 private:
+  mutable PropagateToMuonInterface propInt_;


Suggested change

mutable PropagateToMuonInterface propInt_;

const PropagateToMuonInterface propInt_;

I didn't see the need to use mutable anymore (here and in other similar places). No?

True, that was an oversight, fixed.

missirol · 2022-03-09T22:23:31Z

HLTrigger/Muon/plugins/HLTMuonL3PreFilter.cc

+  desc.add<bool>("useSimpleGeometry", true);
+  desc.add<bool>("useStation2", true);
+  desc.add<string>("useTrack", "tracker");
+  desc.add<string>("useState", "atVertex");
+  desc.add<edm::ESInputTag>("propagatorAlong", edm::ESInputTag("", "hltESPSteppingHelixPropagatorAlong"));
+  desc.add<edm::ESInputTag>("propagatorAny", edm::ESInputTag("", "SteppingHelixPropagatorAny"));
+  desc.add<edm::ESInputTag>("propagatorOpposite", edm::ESInputTag("", "hltESPSteppingHelixPropagatorOpposite"));


To factorise better, this is probably a good candidate for PropagateToMuonInterface::fillPSetDescription, so the lines here would become a single call to that static function (there are various examples of fillPSetDescription in CMSSW).

cmsbuild · 2022-03-10T15:49:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28787

This PR adds an extra 36KB to repository

JanFSchulte · 2022-03-21T16:35:21Z

I won't get around to resolving the duplication immediately, so I'll create an issue for the moment.

perrotta · 2022-03-21T16:39:26Z

please test

cmsbuild · 2022-03-21T16:42:28Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37180/28921

This PR adds an extra 68KB to repository

cmsbuild · 2022-03-21T16:42:55Z

Pull request #37180 was updated. @rekovic, @epalencia, @emanueleusai, @ahmad3213, @Martin-Grunewald, @missirol, @jfernan2, @pmandrik, @santocch, @cecilecaillol, @pbo0, @rvenditti can you please check and sign again.

cmsbuild · 2022-03-22T00:06:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9dc230/23256/summary.html
COMMIT: 972fe57
CMSSW: CMSSW_12_4_X_2022-03-21-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37180/23256/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-9dc230/39434.911_TTbar_14TeV+2026D88_DD4hep+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3695650
DQMHistoTests: Total failures: 51214
DQMHistoTests: Total nulls: 64
DQMHistoTests: Total successes: 3644350
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: found differences in 1 / 48 workflows

perrotta · 2022-03-22T05:43:44Z

+1

This PR was already fully signed, and the last update only cleaned it up from a few unused header include: it can be merged now without waiting for it being signed once again by all categories involved

perrotta · 2022-03-22T05:44:17Z

merge

missirol · 2022-03-22T06:03:49Z

+hlt

@JanFSchulte , please open the backport PR to 12_3_X.

jfernan2 · 2022-03-22T16:39:38Z

+1
For the records

santocch · 2022-03-25T11:35:08Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Mar 8, 2022

cmsbuild added analysis-pending code-checks-pending dqm-pending hlt-pending l1-pending orp-pending pending-signatures tests-pending labels Mar 8, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 8, 2022

Martin-Grunewald reviewed Mar 9, 2022

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 9, 2022

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Mar 9, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 9, 2022

missirol reviewed Mar 9, 2022

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 10, 2022

cmsbuild added l1-pending tests-pending labels Mar 21, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 21, 2022

JanFSchulte mentioned this pull request Mar 21, 2022

Duplication of PropagateToMuon class in MuonAnalysis/MuonAssociators and HLTriggerOffline/Muon #37290

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 21, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 22, 2022

cmsbuild added orp-approved and removed orp-pending labels Mar 22, 2022

cmsbuild merged commit 6c53647 into cms-sw:master Mar 22, 2022

cmsbuild added hlt-approved and removed hlt-pending labels Mar 22, 2022

JanFSchulte mentioned this pull request Mar 22, 2022

Backport the refactoring of the PropagateToMuon class and add the propagating to some HLT filters #37299

Merged

cmsbuild added dqm-approved and removed dqm-pending labels Mar 22, 2022

This was referenced Mar 23, 2022

[ROOT626] Updated root to tip of branch v6-26-00-patches cms-sw/cmsdist#7706

Merged

[ROOT6] Updated root to tip of branch master cms-sw/cmsdist#7705

Merged

Make the dyld-based library search behavior default for non-modules. cms-sw/root#165

Closed

	const Propagator propagatorBarrel = &propagator_;
	const Propagator propagatorEndcaps = &propagator_;
	if (whichState_ != AtVertex) {
	if (start.position().perp() > barrelCylinder_->radius())
	propagatorBarrel = &*propagatorOpposite_;
	if (fabs(start.position().z()) > endcapDiskPos_[useMB2_ ? 2 : 1]->position().z())
	propagatorEndcaps = &*propagatorOpposite_;
	}
	if (cosmicPropagation_) {
	if (start.momentum().dot(GlobalVector(start.position().x(), start.position().y(), start.position().z())) < 0) {
	// must flip the propagations
	propagatorBarrel = (propagatorBarrel == &propagator_ ? &propagatorOpposite_ : &*propagator_);
	propagatorEndcaps = (propagatorEndcaps == &propagator_ ? &propagatorOpposite_ : &*propagator_);
	}
	}

	"HLTriggerOffline/Muon/interface/PropagateToMuonCore.h" \brief Propagate an
	\brief Propagate an

		#ifndef HLTriggerOffline_Muon_interface_trackStateEnums_h
		#define HLTriggerOffline_Muon_interface_trackStateEnums_h

		Chi2MeasurementEstimator estimator(1e10,
		3.); // require compatibility at 3 sigma

	mutable PropagateToMuonInterface propInt_;
	const PropagateToMuonInterface propInt_;

Conversation

JanFSchulte commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmsbuild commented Mar 8, 2022

Uh oh!

cmsbuild commented Mar 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmsbuild commented Mar 9, 2022

Uh oh!

cmsbuild commented Mar 9, 2022

Uh oh!

cmsbuild commented Mar 9, 2022

Uh oh!

JanFSchulte commented Mar 9, 2022

Uh oh!

missirol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmsbuild commented Mar 10, 2022

Uh oh!

JanFSchulte commented Mar 21, 2022

Uh oh!

perrotta commented Mar 21, 2022

Uh oh!

cmsbuild commented Mar 21, 2022

Uh oh!

cmsbuild commented Mar 21, 2022

Uh oh!

cmsbuild commented Mar 22, 2022

Comparison Summary

Uh oh!

perrotta commented Mar 22, 2022

Uh oh!

perrotta commented Mar 22, 2022

Uh oh!

missirol commented Mar 22, 2022

Uh oh!

jfernan2 commented Mar 22, 2022

Uh oh!

JanFSchulte commented Mar 8, 2022 •

edited

Loading